Add support for API 1.48 #4668
Add support for API 1.48 #4668
Conversation
WalkthroughAdds exported constant Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI
participant SerialBackend
participant MSP
participant Dialog
UI->>SerialBackend: connect / onOpen
SerialBackend->>MSP: request board info & apiVersion
MSP-->>SerialBackend: board info & apiVersion
SerialBackend->>SerialBackend: semver check vs CONFIGURATOR.API_VERSION_MAX_SUPPORTED
alt API incompatible
SerialBackend->>Dialog: showVersionMismatchAndCli()
Dialog-->>UI: present modal (switch to CLI / abort)
alt user chooses CLI
SerialBackend->>MSP: MSP.clearListeners()
SerialBackend->>MSP: MSP.disconnect_cleanup()
SerialBackend->>SerialBackend: connectCli()
else user aborts
SerialBackend->>SerialBackend: abort connection / cleanup
end
else API compatible
SerialBackend->>SerialBackend: await checkReportProblems()
SerialBackend->>MSP: request status / problem checks
MSP-->>SerialBackend: status / problems
alt problems detected
SerialBackend->>Dialog: dialogReportProblems()
Dialog-->>UI: show modal (may route to CLI)
else no problems
SerialBackend->>SerialBackend: continue normal open flow
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/js/serial_backend.js (2)
320-335: Avoid accumulating click handlers; make helper reusable.dialogConnectWarning-closebtn uses .click, which stacks handlers across connections. Use .off().one() to prevent leaks. Consider hoisting this helper to module scope and reuse it for all mismatch cases.
- $(".dialogConnectWarning-closebtn").click(function () { - dialog.close(); - }); + $(".dialogConnectWarning-closebtn") + .off("click") + .one("click", function () { + dialog.close(); + });
372-372: Good: unified mismatch handling for non-BTFLUsing dialogConnectVersionMismatch here improves consistency. Consider also replacing the later inline mismatch block to reuse the same helper and reduce duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial_backend.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (3)
src/js/data_storage.js (3)
CONFIGURATOR(8-39)API_VERSION_1_46(4-4)API_VERSION_1_46(4-4)src/js/msp.js (1)
MSP(5-455)src/js/sensor_helpers.js (1)
have_sensor(4-22)
🔇 Additional comments (2)
src/js/serial_backend.js (2)
472-475: Confirm custom-defaults gating for >=1.46processBoardInfo now only calls processCustomDefaults when API < 1.46. For >=1.46, ensure the connection flow still advances (UID, RTC, finishOpen) exactly once and in the intended order, given the earlier call to checkReportProblems.
650-653: Good MSP cleanup before switching to CLIClearing listeners and decoder state avoids stray MSP callbacks in CLI mode. Matches read_serial’s CLI path behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/js/serial_backend.js (4)
482-482: Missing await on MSP.promiseLine 482 calls
MSP.promise(MSPCodes.MSP_STATUS)without awaiting it. This meansFC.CONFIG.configurationProblemsandFC.CONFIG.activeSensorsmay not be updated when the checks run on the following lines.Apply this diff:
- MSP.promise(MSPCodes.MSP_STATUS); + await MSP.promise(MSPCodes.MSP_STATUS);As per previous review comments.
490-490: Use full semver comparisonLine 490 uses
semver.minor()to compare only the minor version parts. This is incorrect for full semantic version comparison. Usesemver.gt()instead.Apply this diff:
- if (semver.minor(FC.CONFIG.apiVersion) > semver.minor(CONFIGURATOR.API_VERSION_MAX_SUPPORTED)) { + if (semver.gt(FC.CONFIG.apiVersion, CONFIGURATOR.API_VERSION_MAX_SUPPORTED)) {As per previous review comments.
519-519: Remove duplicated ID from cloned elementsLine 519 clones the template without clearing its
idattribute, causing duplicate IDs in the DOM when multiple problems are displayed.Apply this diff:
- problemItemTemplate.clone().html(problem.description).appendTo(problemDialogList); + problemItemTemplate.clone().prop("id", null).html(problem.description).appendTo(problemDialogList);As per previous review comments.
523-525: Prevent close handler accumulationLines 523-525 use
.click()to bind the close handler, which accumulates handlers if the dialog is shown multiple times. Use.off().one()instead.Apply this diff:
- $("#dialogReportProblems-closebtn").click(function () { - problemDialog.close(); - }); + $("#dialogReportProblems-closebtn") + .off("click") + .one("click", function () { + problemDialog.close(); + });As per previous review comments.
🧹 Nitpick comments (1)
src/js/serial_backend.js (1)
293-296: Use.one()for single-use close handlerReplace
.off("click").on("click", ...)with.one("click", ...)since the close button only needs to be clicked once per dialog display.Apply this diff:
- $(".dialogConnectWarning-closebtn") - .off("click") - .on("click", function () { - dialog.close(); - }); + $(".dialogConnectWarning-closebtn").one("click", function () { + dialog.close(); + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial_backend.js(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (3)
src/js/data_storage.js (3)
CONFIGURATOR(8-39)API_VERSION_1_46(4-4)API_VERSION_1_46(4-4)src/js/msp.js (1)
MSP(5-455)src/js/sensor_helpers.js (1)
have_sensor(4-22)
🔇 Additional comments (4)
src/js/serial_backend.js (4)
348-359: Double-modal issue resolvedThe previous concern about double-modals has been addressed.
checkReportProblems()now shows its own dialog when version is incompatible and returnsabort=true, then the code simply callsconnectCli()without opening a second modal. This is a cleaner flow.
378-378: LGTM: Good consolidationReplacing inline version mismatch handling with the centralized
showVersionMismatchAndCli()function improves consistency and maintainability.Also applies to: 387-387
466-468: LGTM: Clearer conditionInverting the condition to
semver.ltmakes the intent clearer: custom defaults are only processed for API versions below 1.46.
644-646: LGTM: Proper MSP cleanup before CLIAdding
MSP.clearListeners()andMSP.disconnect_cleanup()ensures a clean state when switching to CLI mode, preventing MSP protocol interference with CLI communication.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/serial_backend.js (1)
477-536: Critical: Missing await and incorrect semver comparison; also fix duplicate IDs and handler accumulation.Several issues need to be addressed:
Missing await (Line 480):
MSP.promise(MSPCodes.MSP_STATUS)must be awaited before readingFC.CONFIG.apiVersion,FC.CONFIG.configurationProblems, orFC.CONFIG.activeSensors.Incorrect semver comparison (Line 488): Comparing only minor versions with
semver.minor()is incorrect. Usesemver.gt(FC.CONFIG.apiVersion, CONFIGURATOR.API_VERSION_MAX_SUPPORTED)for full version comparison.Duplicate IDs (Line 517): Cloning the template preserves its
idattribute, creating duplicate IDs in the DOM. Call.prop("id", null)after cloning.Handler accumulation (Line 521): Using
.click()registers a new handler each time, accumulating handlers. Use.off("click").one("click", ...)instead.Variable declaration (Line 485):
problemsis never reassigned, so it should beconstinstead oflet.Apply this diff:
async function checkReportProblems() { const problemItemTemplate = $("#dialogReportProblems-listItemTemplate"); - MSP.promise(MSPCodes.MSP_STATUS); + await MSP.promise(MSPCodes.MSP_STATUS); let needsProblemReportingDialog = false; const problemDialogList = $("#dialogReportProblems-list"); problemDialogList.empty(); - let problems = []; + const problems = []; let abort = false; - if (semver.minor(FC.CONFIG.apiVersion) > semver.minor(CONFIGURATOR.API_VERSION_MAX_SUPPORTED)) { + if (semver.gt(FC.CONFIG.apiVersion, CONFIGURATOR.API_VERSION_MAX_SUPPORTED)) { const problemName = "API_VERSION_MAX_SUPPORTED"; problems.push({ name: problemName, description: i18n.getMessage(`reportProblemsDialog${problemName}`, [ CONFIGURATOR.latestVersion, CONFIGURATOR.latestVersionReleaseUrl, CONFIGURATOR.getDisplayVersion(), FC.CONFIG.flightControllerVersion, ]), }); needsProblemReportingDialog = true; abort = true; } if (!abort) { // only check for more problems if we are not already aborting needsProblemReportingDialog = checkReportProblem("MOTOR_PROTOCOL_DISABLED", problems) || needsProblemReportingDialog; if (have_sensor(FC.CONFIG.activeSensors, "acc")) { needsProblemReportingDialog = checkReportProblem("ACC_NEEDS_CALIBRATION", problems) || needsProblemReportingDialog; } } if (needsProblemReportingDialog) { for (const problem of problems) { - problemItemTemplate.clone().html(problem.description).appendTo(problemDialogList); + problemItemTemplate.clone().prop("id", null).html(problem.description).appendTo(problemDialogList); } const problemDialog = $("#dialogReportProblems")[0]; - $("#dialogReportProblems-closebtn").click(function () { - problemDialog.close(); - }); + $("#dialogReportProblems-closebtn") + .off("click") + .one("click", function () { + problemDialog.close(); + }); problemDialog.showModal(); $("#dialogReportProblems").scrollTop(0); $("#dialogReportProblems-closebtn").focus(); } if (!abort) { // if we are not aborting, we can continue processUid(); } return abort; }
🧹 Nitpick comments (1)
src/js/serial_backend.js (1)
639-647: LGTM! MSP cleanup is necessary when switching to CLI.The addition of
MSP.clearListeners()andMSP.disconnect_cleanup()is correct and necessary to prevent MSP message handlers from interfering with CLI operations. This aligns with the cleanup pattern used inread_serial()(line 732-733).Consider removing the comment on line 642 since the cleanup is indeed needed:
CONFIGURATOR.connectionValid = true; // making it possible to open the CLI tab GUI.allowedTabs = ["cli"]; - // do we need to cleanup MSP listeners here? MSP.clearListeners(); MSP.disconnect_cleanup(); onConnect();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial_backend.js(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (3)
src/js/data_storage.js (1)
CONFIGURATOR(8-39)src/js/msp.js (1)
MSP(5-455)src/js/sensor_helpers.js (1)
have_sensor(4-22)
🔇 Additional comments (3)
src/js/serial_backend.js (3)
285-301: LGTM! Clean consolidation of version mismatch handling.The new helper function properly centralizes the version mismatch warning dialog and CLI transition logic. Using
.one("click")for the close handler prevents accumulation of event listeners.
338-387: Good solution for avoiding double-modals.The async pre-check pattern properly addresses the previous concern about multiple modals appearing. When
checkReportProblems()indicates an abort, the code switches to CLI without showing another dialog. The use ofshowVersionMismatchAndCli()helper in both branches (lines 376, 385) improves consistency.
464-466: LGTM! Correct API version threshold logic.The inverted check properly handles the version-specific flow for custom defaults processing in older firmware versions.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial_backend.js(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (5)
src/js/data_storage.js (3)
CONFIGURATOR(8-39)API_VERSION_1_46(4-4)API_VERSION_1_46(4-4)src/js/fc.js (1)
FC(132-1006)src/js/msp.js (1)
MSP(5-455)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/sensor_helpers.js (1)
have_sensor(4-22)
🔇 Additional comments (3)
src/js/serial_backend.js (3)
350-362: LGTM! Sequential checks prevent double-modals.The refactored flow correctly separates version compatibility from problem reporting. By checking version first (line 351) and only proceeding to problem checks (line 357) when compatible, the code ensures only one dialog appears at a time.
470-472: LGTM! Inverted condition correctly targets older firmware.Using
semver.ltto callprocessCustomDefaultsonly for API versions below 1.46.0 is appropriate, as the custom defaults flow is relevant for older firmware versions.
628-630: LGTM! Proper MSP cleanup before CLI mode.Calling
MSP.clearListeners()andMSP.disconnect_cleanup()ensures a clean state transition by removing message listeners and clearing pending callbacks before entering CLI mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/serial_backend.js (1)
621-623: LGTM! MSP cleanup before CLI entry prevents stale listeners.Clearing MSP listeners and performing disconnect cleanup ensures the CLI has a clean communication channel without interference from configuration-mode handlers.
The comment "do we need to cleanup MSP listeners here?" is now outdated since the cleanup is implemented. Consider removing or updating it to something like "Clear MSP listeners before entering CLI mode."
Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial_backend.js(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:08:55.679Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/utils/compareVersions.js:3-17
Timestamp: 2025-08-27T22:08:55.679Z
Learning: In betaflight-configurator's compareVersions utility (src/js/utils/compareVersions.js), the behavior of treating non-numeric segments as NaN -> 0 is intentional by design, not a bug that needs fixing.
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (4)
src/js/data_storage.js (1)
CONFIGURATOR(8-39)src/js/msp.js (1)
MSP(5-455)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/sensor_helpers.js (1)
have_sensor(4-22)
🔇 Additional comments (5)
src/js/serial_backend.js (5)
285-301: LGTM! Effective consolidation of version mismatch handling.The centralized
showVersionMismatchAndCli()helper cleanly replaces multiple inline dialog invocations and consistently routes to CLI when the firmware version is unsupported.
303-307: LGTM! Version compatibility check correctly implements major.minor comparison.The use of
semver.satisfies()with the<=${maxMajor}.${maxMinor}range properly enforces major.minor compatibility while ignoring patch versions, as intended.
352-356: LGTM! Pre-flight version check prevents incompatible connections.Checking API version compatibility before proceeding with firmware-specific calls ensures a clean abort path for unsupported versions.
376-376: LGTM! Consistent use of centralized version mismatch handler.Replacing inline dialogs with
showVersionMismatchAndCli()improves maintainability and ensures uniform UX across different incompatibility scenarios.Also applies to: 385-385
479-515: LGTM! Problem reporting flow correctly implements async checks and DOM hygiene.The refactored implementation properly:
- Awaits
MSP_STATUSbefore accessingFC.CONFIG- Strips cloned template IDs to avoid duplicates
- Uses
.off().one()to prevent handler accumulation- Advances the connection flow internally after checks complete
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/serial_backend.js (1)
285-300: Centralized version warning flow looks good.The refactoring to create
showVersionMismatchAndCli()as a centralized helper is a solid improvement. The use of.one("click", ...)correctly prevents handler accumulation.One minor suggestion: at line 353, when this function is called without a message parameter, the default message references
API_VERSION_ACCEPTED(minimum supported version), but the check at line 352 is againstAPI_VERSION_MAX_SUPPORTED(maximum supported version). For the "firmware too new" case, consider passing an explicit message that references the maximum version to avoid user confusion:if (!checkApiVersionCompatibility()) { showVersionMismatchAndCli( i18n.getMessage("firmwareVersionTooNew", [CONFIGURATOR.API_VERSION_MAX_SUPPORTED]) ); return; }(Assuming you have or can add a
firmwareVersionTooNewi18n key.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial_backend.js(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:08:55.679Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/utils/compareVersions.js:3-17
Timestamp: 2025-08-27T22:08:55.679Z
Learning: In betaflight-configurator's compareVersions utility (src/js/utils/compareVersions.js), the behavior of treating non-numeric segments as NaN -> 0 is intentional by design, not a bug that needs fixing.
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (2)
src/js/data_storage.js (1)
CONFIGURATOR(8-39)src/js/sensor_helpers.js (1)
have_sensor(4-22)
🔇 Additional comments (5)
src/js/serial_backend.js (5)
302-306: API version compatibility check correctly implemented.The major.minor version comparison using
semver.satisfies()is correctly implemented and matches the maintainer's intent to ignore patch levels. This addresses the previous review feedback.
351-355: Early version compatibility check improves flow.Adding the compatibility check immediately after receiving the API version is the right approach. This prevents unnecessary MSP calls when the firmware version is incompatible and provides clearer feedback to the user.
375-375: Consistent use of centralized warning helper.Replacing inline dialog code with calls to
showVersionMismatchAndCli()reduces duplication and maintains consistent UX across different connection failure scenarios.Also applies to: 384-384
478-513: Problem reporting refactor successfully addresses all previous feedback.The async refactor and improvements to
checkReportProblems()correctly implement all suggestions from previous reviews:
- ✅ Awaits MSP_STATUS (line 479) before accessing
FC.CONFIGproperties- ✅ Removes duplicate IDs (line 500) with
.prop("id", null)after cloning- ✅ Prevents handler accumulation (lines 504-506) with
.off("click").one("click", ...)- ✅ Separates version checking by moving it to
checkApiVersionCompatibility()The simplified flow that always calls
processUid()at the end is correct, as the problem dialog is informational and doesn't block the connection handshake.
621-622: MSP cleanup before CLI ensures clean transition.Adding
MSP.clearListeners()andMSP.disconnect_cleanup()before activating CLI mode is correct. This ensures that any listeners and callbacks from the connection handshake are cleared, preventing interference with CLI's different communication protocol.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
locales/en/messages.json(1 hunks)src/js/serial_backend.js(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/msp/MSPHelper.js:807-815
Timestamp: 2025-08-27T20:04:29.126Z
Learning: The firmware version transmission format: Legacy firmware sends [MSP_PROTOCOL_VERSION=1, API_VERSION_MAJOR=4, API_VERSION_MINOR=5] for version 4.5.0, while new firmware sends [FC_VERSION_MAJOR=25, FC_VERSION_MINOR=12, FC_VERSION_PATCH_LEVEL=0] for version 25.12.0. The parsing logic correctly detects legacy vs new format by checking if the first byte equals 1.
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:08:55.679Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/utils/compareVersions.js:3-17
Timestamp: 2025-08-27T22:08:55.679Z
Learning: In betaflight-configurator's compareVersions utility (src/js/utils/compareVersions.js), the behavior of treating non-numeric segments as NaN -> 0 is intentional by design, not a bug that needs fixing.
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (6)
src/js/fc.js (1)
FC(132-1006)src/js/data_storage.js (1)
CONFIGURATOR(8-39)src/js/localization.js (1)
i18n(7-7)src/js/msp.js (1)
MSP(5-455)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/sensor_helpers.js (1)
have_sensor(4-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
locales/en/messages.json(1 hunks)src/js/data_storage.js(1 hunks)src/js/serial_backend.js(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-09-19T20:41:44.286Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:07:26.353Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/data_storage.js:9-9
Timestamp: 2025-08-27T22:07:26.353Z
Learning: In the betaflight-configurator CalVer refactoring, the generateVirtualApiVersions function in src/js/utils/common.js works correctly despite the apparent mathematical issue with bounds logic where compareVersions.minor() returns different ranges for API_VERSION_MAX_SUPPORTED (25.12.0) vs API_VERSION_ACCEPTED (1.44.0).
Applied to files:
src/js/serial_backend.js
📚 Learning: 2025-08-27T22:08:55.679Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/utils/compareVersions.js:3-17
Timestamp: 2025-08-27T22:08:55.679Z
Learning: In betaflight-configurator's compareVersions utility (src/js/utils/compareVersions.js), the behavior of treating non-numeric segments as NaN -> 0 is intentional by design, not a bug that needs fixing.
Applied to files:
src/js/serial_backend.js
🧬 Code graph analysis (1)
src/js/serial_backend.js (3)
src/js/data_storage.js (1)
CONFIGURATOR(8-39)src/js/msp.js (1)
MSP(5-455)src/js/sensor_helpers.js (1)
have_sensor(4-22)
🔇 Additional comments (5)
src/js/data_storage.js (1)
11-11: Clarify the apparent inconsistency between PR title and MAX_SUPPORTED version.The PR is titled "Add support for API 1.48", yet
API_VERSION_MAX_SUPPORTEDis set toAPI_VERSION_1_46. According to the AI summary, this was previouslyAPI_VERSION_1_47, meaning this change actually lowers the maximum supported version. This seems contradictory to the PR's stated objective.Is this intentional? If so, please clarify:
- Why is MAX_SUPPORTED being lowered to 1.46?
- What is the purpose of adding the
API_VERSION_1_48constant if it's above the max supported version?- Is this a temporary change or is there a specific compatibility reason?
src/js/serial_backend.js (4)
285-295: LGTM! Well-structured centralized version warning handler.The
showVersionMismatchAndClihelper effectively consolidates version mismatch handling into a single path, ensuring consistent UX and proper cleanup with.one("click")to prevent handler accumulation.
340-353: LGTM! Version compatibility check properly positioned in the connection flow.The early version check using
semver.satisfieswith major.minor comparison (ignoring patch) prevents incompatible connections before attempting further handshake steps. This avoids the double-modal issues that were present in earlier iterations.Based on past review discussions, the major.minor-only comparison is intentional.
473-509: LGTM! Problem reporting properly refactored with async flow and cleanup fixes.The async implementation correctly awaits
MSP_STATUS, clones problem items with.prop("id", null)to prevent duplicate IDs, and uses.off("click").one("click")to prevent handler accumulation. All issues from previous reviews have been addressed.
616-617: LGTM! Proper MSP cleanup before CLI switch.Adding
MSP.clearListeners()andMSP.disconnect_cleanup()before opening the CLI ensures a clean transition and prevents stale listeners or state from interfering with CLI operations.
|
|
Preview URL: https://pr4668.betaflight-app-preview.pages.dev |
nerdCopter
left a comment
nerdCopter
left a comment
There was a problem hiding this comment.
f34313b
connects, flashes, cli-tab takes a paste. nothing else tested.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Localization